fix(katana): prevent from resending messages to L1 #2354
Conversation
WalkthroughThe changes introduce a new Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- bin/sozo/src/commands/mod.rs (1 hunks)
- crates/dojo-lang/src/inline_macros/get.rs (1 hunks)
- crates/katana/core/src/backend/storage.rs (3 hunks)
- crates/katana/core/src/service/messaging/service.rs (5 hunks)
- crates/katana/primitives/src/genesis/json.rs (4 hunks)
- crates/katana/primitives/src/genesis/mod.rs (3 hunks)
- crates/katana/storage/db/src/tables.rs (5 hunks)
- crates/katana/storage/provider/src/error.rs (1 hunks)
- crates/katana/storage/provider/src/lib.rs (2 hunks)
- crates/katana/storage/provider/src/providers/db/mod.rs (2 hunks)
- crates/katana/storage/provider/src/providers/fork/mod.rs (2 hunks)
- crates/katana/storage/provider/src/providers/in_memory/cache.rs (2 hunks)
- crates/katana/storage/provider/src/providers/in_memory/mod.rs (2 hunks)
- crates/katana/storage/provider/src/traits/messaging.rs (1 hunks)
- crates/katana/storage/provider/src/traits/mod.rs (1 hunks)
Files skipped from review due to trivial changes (3)
- bin/sozo/src/commands/mod.rs
- crates/dojo-lang/src/inline_macros/get.rs
- crates/katana/storage/provider/src/error.rs
Additional comments not posted (36)
crates/katana/storage/provider/src/traits/mod.rs (1)
4-4: LGTM!The addition of the
messagingmodule enhances the modular design of the codebase.The code changes are approved.
crates/katana/storage/provider/src/traits/messaging.rs (4)
1-2: LGTM!The import of
BlockNumberis appropriate and necessary for the trait methods.The code changes are approved.
3-4: LGTM!The import of
ProviderResultis appropriate and necessary for the trait methods.The code changes are approved.
5-6: LGTM!The constants
SEND_FROM_BLOCK_KEYandGATHER_FROM_BLOCK_KEYare well-defined and appropriately named.The code changes are approved.
8-18: LGTM!The
MessagingProvidertrait is well-defined with appropriate methods for setting and getting block numbers. The use ofauto_implis a good practice for flexibility.The code changes are approved.
crates/katana/storage/provider/src/providers/in_memory/cache.rs (2)
88-88: LGTM!The addition of the
messaging_infofield enhances the data structure by allowing it to store additional messaging-related information associated with block numbers.The code changes are approved.
121-121: LGTM!The initialization of the
messaging_infofield with a default value ensures that the field is properly set up during the instantiation of the struct.The code changes are approved.
crates/katana/storage/provider/src/lib.rs (5)
19-19: LGTM!The import of
MessagingProvideris necessary for the implementation.The code changes are approved.
384-387: LGTM!The implementation of
MessagingProviderforBlockchainProvider<Db>is necessary to integrate messaging capabilities.The code changes are approved.
388-390: LGTM!The
get_send_from_blockmethod correctly delegates the call to the underlying provider.The code changes are approved.
392-394: LGTM!The
set_send_from_blockmethod correctly delegates the call to the underlying provider.The code changes are approved.
396-402: LGTM!The
get_gather_from_blockandset_gather_from_blockmethods correctly delegate the calls to the underlying provider.The code changes are approved.
crates/katana/core/src/backend/storage.rs (2)
12-12: LGTM!The addition of
MessagingProviderto theDatabasetrait is necessary to ensure that any type implementingDatabasealso implementsMessagingProvider.The code changes are approved.
Also applies to: 35-35
56-56: LGTM!The addition of
MessagingProviderto the implementation ofDatabasefor generic types is necessary to ensure that all types conforming toDatabasealso adhere to theMessagingProviderrequirement.The code changes are approved.
crates/katana/core/src/service/messaging/service.rs (3)
12-12: LGTM!The import of
MessagingProvideris necessary for the modifications in theMessagingServiceimplementation.The code changes are approved.
50-70: LGTM!The retrieval of
gather_from_blockandsend_from_blockfrom the provider, along with the error handling, ensures that the messaging service operates with the correct block numbers and fails fast in case of misconfiguration.The code changes are approved.
225-229: LGTM!The calls to
set_gather_from_blockandset_send_from_blockensure that the provider is updated with the latest block numbers, improving synchronization between the messaging service and the blockchain.The code changes are approved.
Also applies to: 255-256
crates/katana/storage/db/src/tables.rs (4)
47-47: LGTM!The
NUM_TABLESconstant is correctly updated to reflect the addition of a new table.The code changes are approved.
170-171: LGTM!The
MessagingInfotable is correctly added to thedefine_tables_enum!macro.The code changes are approved.
229-230: LGTM!The
MessagingInfotable is correctly added to thetables!macro.The code changes are approved.
264-264: LGTM!The test module is correctly updated to include assertions for the
MessagingInfotable.The code changes are approved.
Also applies to: 289-289
crates/katana/storage/provider/src/providers/in_memory/mod.rs (3)
29-29: LGTM!The
MessagingProvidertrait and related constants are correctly imported.The code changes are approved.
571-571: LGTM!The
MessagingProvidertrait is correctly implemented forInMemoryProvider.The code changes are approved.
572-589: LGTM!The methods to get and set
send_from_blockandgather_from_blockare correctly implemented.The code changes are approved.
crates/katana/storage/provider/src/providers/fork/mod.rs (3)
33-33: LGTM!The
MessagingProvidertrait is correctly imported.The code changes are approved.
577-577: LGTM!The
MessagingProvidertrait is correctly implemented forForkedProvider.The code changes are approved.
578-593: LGTM!The methods to get and set
send_from_blockandgather_from_blockare correctly implemented.The code changes are approved.
crates/katana/primitives/src/genesis/mod.rs (3)
103-104: LGTM!The addition of the
gather_from_blockfield enhances the functionality of theGenesisstruct by enabling it to track the specific block for message retrieval.The code changes are approved.
300-300: LGTM!The update to the
Defaultimplementation ensures that any instance ofGenesiscreated using the default constructor will have a defined starting point for message gathering.The code changes are approved.
416-416: LGTM!The update to the test module ensures that the new
gather_from_blockfield is accounted for in both the default behavior and the testing scenarios, thereby maintaining consistency across the implementation.The code changes are approved.
crates/katana/storage/provider/src/providers/db/mod.rs (4)
750-750: LGTM!The implementation of the
MessagingProvidertrait allows theDbProviderto manage messaging-related functionalities, enhancing its role in the broader application architecture.The code changes are approved.
751-756: LGTM!The
get_send_from_blockmethod correctly retrieves the block number associated with theSEND_FROM_BLOCK_KEYfrom the database transaction, ensuring proper transaction handling with commit operations.The code changes are approved.
758-763: LGTM!The
set_send_from_blockmethod correctly updates the database with a new block number for theSEND_FROM_BLOCK_KEY, encapsulated in a closure that manages the database transaction.The code changes are approved.
765-777: LGTM!The
get_gather_from_blockandset_gather_from_blockmethods correctly handle the retrieval and updating of the block number for theGATHER_FROM_BLOCK_KEYin the database, ensuring proper transaction handling with commit operations.The code changes are approved.
crates/katana/primitives/src/genesis/json.rs (2)
219-219: LGTM!The
gather_from_blockfield is correctly added to theGenesisJsonstruct.The code changes are approved.
476-476: LGTM!The
gather_from_blockfield is correctly added to theimpl TryFrom<GenesisJson> for Genesis.The code changes are approved.
| Ok(Some(block)) => block, | ||
| Ok(None) => 0, | ||
| Err(_) => { | ||
| panic!( |
There was a problem hiding this comment.
Prefer an error instead of panicking for all the one present in this function, and instead propagate in the new to ensure error is correctly displayed.
| impl<Db> MessagingProvider for BlockchainProvider<Db> | ||
| where | ||
| Db: MessagingProvider, | ||
| { | ||
| fn get_send_from_block(&self) -> ProviderResult<Option<BlockNumber>> { | ||
| self.provider.get_send_from_block() | ||
| } | ||
|
|
||
| fn set_send_from_block(&self, send_from_block: BlockNumber) -> ProviderResult<()> { | ||
| self.provider.set_send_from_block(send_from_block) | ||
| } | ||
|
|
||
| fn get_gather_from_block(&self) -> ProviderResult<Option<BlockNumber>> { | ||
| self.provider.get_gather_from_block() | ||
| } | ||
|
|
||
| fn set_gather_from_block(&self, gather_from_block: BlockNumber) -> ProviderResult<()> { | ||
| self.provider.set_gather_from_block(gather_from_block) | ||
| } | ||
| } |
There was a problem hiding this comment.
As seen, we should also keep track of a pointer on the message.
L1->L2, we can use the nonce of the message.
L2->L1, let's use the index of the message in the block as they are always ordered.
| pub const SEND_FROM_BLOCK_KEY: u64 = 1; | ||
| pub const GATHER_FROM_BLOCK_KEY: u64 = 2; |
There was a problem hiding this comment.
lets remove this. the provider traits are meant to abstract how data are stored in the underlying storage so it doesn't make sense for these values to be presented here. anyway, we don need to use this for in-memory db. this make sense for on-disk db but for in-memory db, a simple struct that stores both values is more than enough.
so lets move this value to the db crate instead. i would also prefer to represent the keys as enum instead.
enum MessagingCheckpointId {
Send,
Gather
}and implement the Encode and Decode traits for it.
| pub const GATHER_FROM_BLOCK_KEY: u64 = 2; | ||
|
|
||
| #[auto_impl::auto_impl(&, Box, Arc)] | ||
| pub trait MessagingProvider: Send + Sync { |
There was a problem hiding this comment.
better to have more descriptive name
| pub trait MessagingProvider: Send + Sync { | |
| pub trait MessagingCheckpointProvider: Send + Sync { |
| impl MessagingProvider for ForkedProvider { | ||
| fn get_send_from_block(&self) -> ProviderResult<Option<BlockNumber>> { | ||
| Ok(None) | ||
| } | ||
|
|
||
| fn set_send_from_block(&self, _send_from_block: BlockNumber) -> ProviderResult<()> { | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn get_gather_from_block(&self) -> ProviderResult<Option<BlockNumber>> { | ||
| Ok(None) | ||
| } | ||
|
|
||
| fn set_gather_from_block(&self, _gather_from_block: BlockNumber) -> ProviderResult<()> { | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
ForkedProvider use the same underlying struct as InMemoryProvider. so can just copy what we did for InMemoryProvider
| @@ -85,6 +85,7 @@ pub struct CacheDb<Db> { | |||
| pub(crate) transaction_hashes: HashMap<TxNumber, TxHash>, | |||
| pub(crate) transaction_numbers: HashMap<TxHash, TxNumber>, | |||
| pub(crate) transaction_block: HashMap<TxNumber, BlockNumber>, | |||
| pub(crate) messaging_info: HashMap<u64, BlockNumber>, | |||
| @@ -216,6 +216,7 @@ pub struct GenesisJson { | |||
| pub accounts: HashMap<ContractAddress, GenesisAccountJson>, | |||
| #[serde(default)] | |||
| pub contracts: HashMap<ContractAddress, GenesisContractJson>, | |||
| pub gather_from_block: BlockNumber, | |||
There was a problem hiding this comment.
i think the genesis is determined by the smart contract in the underlying settlement layer, it also indicates the minimum starting point of when messages should be collected from. so it's more appropriate to rename this field into something like settlement_block_number which implicitly tells the messaging task to start fetching the messages starting from this block number of the L1.
| @@ -100,6 +100,8 @@ pub struct Genesis { | |||
| pub universal_deployer: Option<UniversalDeployerConfig>, | |||
| /// The genesis contract allocations. | |||
| pub allocations: BTreeMap<ContractAddress, GenesisAllocation>, | |||
| /// The block on settlement chain from where Katana will start fetching messages. | |||
| pub gather_from_block: BlockNumber, | |||
There was a problem hiding this comment.
refer to comment on the genesis json.
|
|
||
| /// Stores the block number related to messaging service | ||
| MessagingInfo: (u64) => BlockNumber |
There was a problem hiding this comment.
| MessagingInfo: (u64) => BlockNumber | |
| MessagingCheckpoint: (MessagingCheckpointId) => BlockNumber |
refer to comment below
ybensacq
force-pushed
the
feat-prevent-resending-message
branch
from
4f19ab6 to
fe2c75b
Compare
There was a problem hiding this comment.
Thanks for the work here @ybensacq and sorry for the delay on my end.
Do you see a simple way to add some testing here? Testing messages are not sent twice with an anvil or a katana as base layer could be great.
You may actually check the e2e testing, where we could have Katana being stopped and restarted, verifying that no messages are actually processed?
| // get stored nonce from message hash | ||
| let message_hash_bytes = l1_tx.message_hash; | ||
| let message_hash_bytes: [u8; 32] = *message_hash_bytes; | ||
|
|
||
| let message_hash = Felt::from_bytes_be(&message_hash_bytes); | ||
| match provider.get_nonce_from_message_hash(message_hash) { | ||
| Ok(Some(nonce)) => provider.set_gather_message_nonce(nonce), | ||
| Ok(None) => Ok(()), | ||
| Err(_e) => Ok(()), | ||
| } |
There was a problem hiding this comment.
Let's move that into a function taking the l1_tx as input. Maybe into the provider directly. Since we're calling provider several time, internalizing the process and having only one function here would make it simpler and easier to test.
| @@ -763,6 +763,78 @@ impl<Db: Database> BlockWriter for DbProvider<Db> { | |||
| } | |||
| } | |||
|
|
|||
| impl MessagingCheckpointProvider for DbProvider { | |||
| fn set_send_from_block(&self, send_from_block: BlockNumber) -> ProviderResult<()> { | |||
There was a problem hiding this comment.
Wasn't those methods renamed from @kariy comments?
When I initially did the messaging, I guess the naming wasn't that good. May need a future rework.
In the meantime, for the function you add, we may attempt a better naming.
The gather is getting messaging from settlement layer and processing them.
The send is sending messaging from katana to the settlement layout.
Concept of inbound, outbound could match well. inbound is from settlement layer to current layer. And outbound is from current layer to settlement layer.
Or we could add the concept of settlement here too. Maybe easier:
set_settlement_block
set_sequencer_block <- this one referencing the katana one?
But maybe inbound and outbound would work nicely.
glihm
changed the title
glihm
changed the title
|
@ybensacq gm sensei, if you don't have the bandwidth, don't hesitate to mention it and will take over. 🫡 Thanks for the work already done on that! |
Description
Related issue
Fixes #2033
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
MessagingProvidertrait to manage messaging operations across various providers.gather_from_blockfield toGenesisandGenesisJsonconfigurations, enabling specific block number tracking.DbProviderandForkedProviderto manage messaging-related block numbers with new methods.CacheDbto include messaging information storage.Bug Fixes
ProviderError.Documentation
Migratecommand description.